⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Known Issues

Tests intentionally excluded from CI. Source of truth for the Known
Issues section the L1 workflow appends to each ef-tests job summary
and posts as a sticky PR comment.

EF Tests — Stateless coverage narrowed to EIP-8025 optional-proofs

make -C tooling/ef_tests/blockchain test calls test-stateless-zkevm
instead of test-stateless. The zkevm@v0.3.3 fixtures are filled against
bal@v5.6.1, out of sync with current bal spec; the broad target trips ~549
fixtures. Re-broaden once the zkevm bundle is regenerated.

Lines of code report

Total lines added: 67
Total lines removed: 5
Total lines changed: 72

+----------------------------------------+-------+------+
| File                                   | Lines | Diff |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs | 2541  | -5   |
+----------------------------------------+-------+------+
| ethrex/crates/vm/backends/levm/mod.rs  | 2515  | +67  |
+----------------------------------------+-------+------+

Benchmark Results Comparison

No significant difference was registered for any benchmark run.

Benchmark Block Execution Results Comparison Against Main

🤖 Claude Code Review

Now I have enough context for a thorough review.

PR Review: perf(l1): reduce BAL parallel-path overhead

Overall Assessment

This is a well-engineered performance bundle with clear, documented reasoning for each optimization. The changes show strong understanding of the BAL parallel execution model. Test coverage is solid (EF tests, Hive, mainnet-mix fixture). A few correctness subtleties warrant attention.

Correctness Concerns

blockchain.rs: Single-recv assumption is fragile (medium severity)

// Line 82
let updates: Vec<AccountUpdate> = match rx.recv() {
    Ok(updates) => { ... updates }
    Err(_) => Vec::new()
};

The change replaces a draining loop with a single recv(), relying on the invariant that execute_block_parallel sends exactly one batch. This is documented in the comment, and verified to be true today (mod.rs:1086-1089). However:

• The contract lives entirely in code comments. If a future refactor sends a second batch (e.g. for incremental streaming), the second batch is silently dropped — no panic, no error, just a truncated state root.
• Consider adding a debug assertion: debug_assert!(rx.try_recv().is_err(), "merkleizer received unexpected second batch") after the recv(). This costs nothing in release builds but would catch protocol drift in CI.

bal_to_account_updates: Fast path 1 removed: false assumption

// ~line 740 (mod.rs)
if !has_info_changes {
    updates.push(AccountUpdate { ..., removed: false, info: None, ... });
    continue;
}

The logic is correct: without info changes, balance/nonce/code_hash are identical in pre- and post-state, so the account cannot transition to empty (removed = post_empty && !pre_empty is always false). The reasoning holds. But the comment ("can't be a removal … by definition") glosses over the "pre-state might already be empty" edge case — it's actually fine because if pre was empty AND storage changes arrived, the pre-empty invariant was already false. Worth noting for future readers.

unread_storage_reads cleanup: semantic equivalence confirmed

The refactoring from per-tx current_state iteration to pre-computed destroyed_addresses/read_keys vecs is semantically identical to the serial loop. The retain(|&(a, _)| a != *addr) call for destroyed accounts is O(n) per destroyed account, same as before — acceptable given destroyed accounts are rare.

Validation error ordering change

In the old serial loop, BAL validation for tx N short-circuited before tx N+1 ran. Now all tx validations execute in parallel, and only the first error (by tx index, since exec_results is sorted) is surfaced after the gas check. This is a behavior-preserving change for the common case, but if validation error messages ever become important for debugging, the silent discard of later errors could be surprising. Consider logging suppressed errors at warn! level.

Correctness Fix (Positive)

Coinbase exemption for 0-tx blocks (mod.rs:1395):

if !unaccessed_pure_accounts.is_empty() && !exec_results.is_empty() {
    unaccessed_pure_accounts.remove(&header.coinbase);

The added && !exec_results.is_empty() guard is a genuine correctness improvement. Empty blocks / withdrawal-only blocks never trigger fee finalization, so a BAL entry for the coinbase in such blocks is legitimately extraneous and should fail validation. This matches the EELS test (test_bal_invalid_extraneous_coinbase) and is correct.

Design / Structural Notes

wait_for_persistence_idle: Rendezvous correctness verified

The Ping-based idle detection is elegant. Confirmed that trie_upd_tx/trie_upd_rx are created with sync_channel(0) (store.rs:1557), so the rendezvous semantics hold: a successful send proves the worker is back at recv(). The only gap is that this invariant isn't enforced at the type level — if someone changes sync_channel(0) to sync_channel(1), Pings could buffer and the method would return a false positive. A comment at the channel creation site pointing to wait_for_persistence_idle would help protect against this:

// INVARIANT: capacity must stay 0 (rendezvous) — wait_for_persistence_idle
// depends on send-blocking to detect worker idle. See Store::wait_for_persistence_idle.
let (trie_upd_tx, trie_upd_rx) = std::sync::mpsc::sync_channel(0);

code_cache index binding (mod.rs:1138–1147 and ~947–967)

code_cache[acct_idx][code_idx] assumes code_cache is built from bal.accounts() in the same order as accounts_by_min_index resolves its .1 field. Both are derived from the same bal.accounts() slice, so this holds today. It's an implicit invariant — a brief comment at the cache[acct_idx] access site noting "acct_idx is an index into bal.accounts()" would make the coupling explicit.

BalAccountCodeCache type name

type BalAccountCodeCache = Vec<(H256, Option<Code>)>;

The name is slightly misleading: it's not a cache for the whole BAL, it's a per-account cache of code-change entries. PerAccountCodeChanges or BallAccountCodeChanges would be more descriptive, though this is minor.

Performance Observations

DashMap migration is well-motivated. The switch from RwLock<FxHashMap> removes the write-lock bottleneck under rayon parallelism. Using FxBuildHasher preserves the fast hash for short fixed-size keys (Address, H256). The TOCTOU double-fetch pattern on cache miss is retained from the old design and is acceptable (idempotent reads from backing store).

PER_TX_DB_CAPACITY = 32 is a reasonable constant. The comment notes "most txs touch <10 accounts" — with a load factor of ~0.87 in hashbrown, capacity 32 → 28 usable entries before a resize. For typical txs this should be sufficient.

or_insert_with(|| FxHashMap::with_capacity_and_hasher(8, ...)) in gen_db.rs: The pre-sizing comment is well-supported by the flamegraph reference. A capacity of 8 means one initial alloc handles the common case of 5–8 SSTOREs per account per call frame cleanly.

Minor Nits

• prefetch_accounts and prefetch_storage now return the try_for_each result directly (no trailing Ok(())). This is idiomatic Rust and correct, but is a style break from the rest of the file which uses explicit Ok(()). Consistent either way.
• The expect("checked by has_full_info_coverage") calls in fast path 2 are safe and the message is useful. Prefer keeping them over unwrap() for diagnosability.
• drop(current_state); drop(codes); on lines 1322-1323 are explicit drops that the comment justifies (to avoid crossing the rayon boundary). The drops are correct but redundant since both are consumed by the validation closure before the Ok((...)) return. Rust would drop them at end-of-scope anyway, but the explicit drops reinforce the intent.

Summary

• Merkleization overlap fix: correct, but the single-recv contract should be guarded with a debug assertion.
• Fast paths in bal_to_account_updates: logically sound; the storage-only fast path corner case around pre-empty accounts is correctly handled.
• TrieMessage::Ping: elegant, correct, but the sync_channel(0) invariant needs protection at the creation site.
• DashMap migration: clean, removes the PoisonError code path, measurably correct.
• Coinbase exemption fix: genuine correctness improvement.
• Code cache: correct, implicit index invariant worth documenting.

Automated review by Claude (Anthropic) · sonnet · custom prompt

🤖 Codex Code Review

No blocking correctness or security findings from static review. The deferred BAL-validation ordering and the trie-worker idle handshake both look coherent.

Non-blocking perf note:

1. crates/vm/backends/levm/mod.rs:747 still calls prefetch_accounts for every write account before the new fast-path split at crates/vm/backends/levm/mod.rs:791. That means storage-only and full-info/non-empty accounts still pay the underlying get_account_state lookup, so the new fast paths mostly avoid a later cache hit rather than the actual DB read. If this PR’s goal is BAL-path latency reduction, filtering prefetches down to slow-path candidates would capture more of the intended win.

Testing gap:

• I couldn’t run the targeted EIP-7928/EIP-8037 test slice here. cargo test is blocked in this environment by read-only ~/.cargo / ~/.rustup state plus an unfetched git dependency (libssz), so this review is source-only.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

Greptile Summary

This PR is a performance bundle for the BAL parallel-execution path rebased onto bal-devnet-7-pr, delivering ~+26% throughput over the unpatched parallel baseline through eight independent improvements.

• Overlap fix (blockchain.rs): Stage A now recv() once instead of draining the channel, allowing Stage B (parallel storage roots) to overlap with still-in-flight rayon execution; FxHashMap merge step dropped since BAL guarantees one entry per address.
• DB contention eliminated (db/mod.rs): RwLock<FxHashMap> caches replaced with DashMap<_, _, FxBuildHasher>, removing ~11% CPU cost from read_contended under 16 rayon workers; prefetch_accounts/prefetch_storage simplified with par_iter + entry.or_insert.
• Per-tx allocator pressure and BAL validation parallelised (levm/mod.rs): GeneralizedDatabase capacity capped at 32, code objects pre-computed once per BAL code change, per-tx validation moved inside the rayon closure with deferred errors surfaced after the gas-limit check.

Note: change B in the PR description (BAL_PARALLEL_TX_THRESHOLD = 5 adaptive fallback) is described but not present in the diff — worth confirming whether it was intentionally dropped during the rebase.

Confidence Score: 4/5

The parallel execution path changes are logically correct and well-tested; the two minor concerns do not affect production block processing.

Core logic is sound. Main risk is the implicit one-batch invariant in handle_merkleization_bal — a future modification sending a second batch would silently produce a wrong state root. A debug_assert after the Ok arm would fully mitigate this.

crates/blockchain/blockchain.rs (single-recv invariant and error-path Stage C) and crates/vm/backends/levm/mod.rs (deferred BAL validation fast paths)

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
crates/blockchain/blockchain.rs:870-882
**Single-recv invariant has no defensive check**

`handle_merkleization_bal` now consumes exactly one message and then never touches `rx` again. If `execute_block_parallel` is ever modified to send a second batch, the extra message will sit in the unbounded channel and be silently dropped — the merkleizer will proceed with only the first batch, producing a wrong state root with no error. A defensive `debug_assert!(rx.try_recv().is_err(), "expected exactly one batch from execute_block_parallel")` after the `Ok` arm would catch any accidental protocol change during development.

### Issue 2 of 3
crates/blockchain/blockchain.rs:876-881
**`Err(_)` path continues through Stage C (16 trie opens)**

When the channel is closed without a message (execution failure before `bal_to_account_updates`), the function returns `Vec::new()` and falls through to Stage C, which unconditionally spawns 16 threads to open the parent state trie even though all shards will have no items. Returning an empty `AccountUpdatesList` early in the `Err` arm would avoid this overhead without changing the visible behaviour, since the execution error surfaces via `execution_result?` regardless.

### Issue 3 of 3
crates/vm/levm/Cargo.toml:23
`dashmap` is added as an inline version requirement rather than routing through the workspace. Nearly every other dep in this file uses `workspace = true`. A workspace entry ensures the version is bumped in one place and keeps the lockfile diff minimal.

```suggestion
dashmap.workspace = true
```

_{Reviews (1): Last reviewed commit: "perf(l1): pre-size backup_storage_slot i..." | Re-trigger Greptile}

Closing. Of the original bundle:

• A (Stage A rx.recv fix) is obsoleted by perf(l1): bal optimistic merkleization (or post-state root optimization) #6655 (validation path deletes Stage A entirely).
• Q1 fast paths' validation-side benefit is obsoleted by perf(l1): bal optimistic merkleization (or post-state root optimization) #6655 (skips bal_to_account_updates on validation path); builder-path benefit remains and may land as a separate PR if benched.
• Q2 cap-32 per-tx DB is already in perf(l1): lazy BAL cursor for per-tx parallel execution #6669.
• Q3 code_from_bal cache is partially obsolete given perf(l1): lazy BAL cursor for per-tx parallel execution #6669; remaining utility lives only on the system-call / outer-seed paths and may be relanded separately.
• Coinbase fix already in main.
• Q8 (move per-tx BAL validation into par_iter closure) and the backup_storage_slot pre-size are still uniquely useful; re-landing as small focused PRs.